Feature/basuruk/nu 009 alias matching, delta apply, refcounted provenance - #47

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

##[group]Run if [ -z "$OCR_LLM_TOKEN" ]; then
 �[36;1mif [ -z "$OCR_LLM_TOKEN" ]; then�[0m
 �[36;1m  echo "::error::OCR_LLM_TOKEN secret is empty; the review cannot authenticate against the LLM endpoint (fork pull requests never receive secrets)"�[0m

##[group]Run if [ -z "$OCR_LLM_TOKEN" ]; then
 �[36;1mif [ -z "$OCR_LLM_TOKEN" ]; then�[0m
 �[36;1m  echo "::error::OCR_LLM_TOKEN secret is empty; the review cannot authenticate against the LLM endpoint (fork pull requests never receive secrets)"�[0m

##[group]Run BASE_REF="${INPUT_BASE_REF:-$EVENT_BASE_REF}"
 �[36;1mBASE_REF="${INPUT_BASE_REF:-$EVENT_BASE_REF}"�[0m
 �[36;1mHEAD_SHA="${INPUT_HEAD_SHA:-$EVENT_HEAD_SHA}"�[0m
 �[36;1mPR_NUMBER="${INPUT_PR_NUMBER:-${EVENT_PR_NUMBER:-$WORKFLOW_RUN_PR_NUMBER}}"�[0m
 �[36;1m# Fail here rather than after the review: everything downstream posts to�[0m
 �[36;1m# this number, and an empty one used to reach GitHub as�[0m
 �[36;1m# /repos/OWNER/REPO/issues//comments — a 404 that discarded a completed�[0m
 �[36;1m# review's findings.�[0m
 �[36;1mif [ -z "$PR_NUMBER" ]; then�[0m
 �[36;1m  echo "::error::No pull request number: the '${GITHUB_EVENT_NAME:-unknown}' event payload carries none. Pass the pr_number input; a workflow_run payload names the PR under workflow_run.pull_requests[0].number."�[0m

Summary by CodeRabbit

• New Features
  • Compiled knowledge graphs are now saved to Neo4j, with content and relationships updated as new versions are processed.
  • Existing graph content is reused when unchanged, and outdated content is removed when no longer referenced.
  • Neo4j’s Bolt connection is available on the local host for the AI service.

Walkthrough

This change adds a Neo4j persistence worker for compiled graphs. It configures host access to Neo4j, applies node and relation updates, tracks node provenance, and processes pending rows with retry and persisted-state handling.

Changes

Compiled graph persistence

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant persist_pending
  participant Postgres
  participant persist_compiled_graph
  participant Neo4jPropertyGraphStore
  participant Embedder
  persist_pending->>Postgres: Claim pending row and load graph data
  persist_pending->>persist_compiled_graph: Persist compiled graph
  persist_compiled_graph->>Neo4jPropertyGraphStore: Load nodes and apply graph deltas
  persist_compiled_graph->>Embedder: Embed changed node content
  persist_compiled_graph->>Neo4jPropertyGraphStore: Upsert resolved relations
  persist_pending->>Postgres: Replace provenance records and mark persisted

Suggested reviewers: basuruifs

Merge Risk: ⚪ Minimal · up to 4793b

The persistence flow connects compiled graphs to Neo4j with provenance reconciliation and retry handling. No outstanding merge-blocking issue is established; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 4793b

Content approval and local-only database access limit exposure. However, the new persistence lifecycle can leave graph data inconsistent with recorded ownership during version retries or overlapping writes, and corrective updates cannot retract obsolete relationships. These limitations weaken evidence integrity and rollback.

Retained concerns

• Medium · reliability · inferred: Persistence retires the immediately preceding approved version without ensuring that version has finished persisting. Claims skip rows in retry backoff and are not serialized by source filename. Consequently, a later version can persist while its predecessor has no provenance records, leaving an older persisted version's contributions untouched; the delayed predecessor can subsequently write obsolete content. This breaks replacement of a source's provenance slice and corrective rollback even with one active worker.
• Medium · reliability · inferred: Node content upsert and provenance attachment are separate committed operations. With overlapping document persists, one writer can upsert a shared node, another can release its last existing reference and delete it, and the first writer's subsequent reference query can match nothing. The result is not checked before PostgreSQL records ownership and marks the document persisted. Per-document leases do not serialize shared-node ownership, so this interleaving can leave durable provenance pointing to a missing node. The documented single-worker assumption limits exposure, but does not enforce exclusion.
• Medium · reliability · observed: Relationships are deliberately only upserted: they have no per-document provenance or removal path. Re-approval can therefore remove an assertion from the source while its old relationship survives whenever both endpoints remain. Deleting a node's last reference cleans incident relationships, but cannot retract an obsolete connection between surviving shared nodes. This is a documented design limitation that prevents corrective updates from reliably withdrawing graph assertions.

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit checks the graph at night,
New nodes spring with vectors bright.
Old refs leave when they are through,
Relations find their endpoints too.
The worker polls, then rests once more,
While tests keep watch beside the door.}

_{Comment @coderabbitai help to get the list of available commands.}

PR Reviewer Guide 🔍

(Review updated until commit 4793b55)

Here are some key observations to aid the review process:

def _content_from_compiled_node(node: dict[str, Any]) -> dict[str, Any]:
    """Strips the per-document provenance dict from a compiled_graph node before comparison.

    Also drops None-valued properties (compiler.py always sets author/author_source/timestamp on
    a DECISION node, even to None when unmatched): Neo4j has no null property value, so a
    round-tripped node never has that key at all (`upsert_nodes`'s own `apoc.map.clean` strips it
    on write, `get()`'s `remove_empty_values` strips it on read). Comparing against a dict that
    still carries `{"author": None}` would never equal the round-tripped one, marking every such
    node "update" forever and re-embedding it on every persist -- the exact spend this module
    exists to avoid.
    """
    extra: dict[str, Any] = {}
    for key, value in node["properties"].items():
        if key == "provenance" or value is None:
            continue
        extra[key] = value
    return _content(node["label"], node["name"], extra)

embedding_text = _embedding_text(content["label"], content["display_name"])
node = EntityNode(
    name=delta.node_key,
    label=content["label"],
    properties=_node_properties(content, model_id, dimensions),
    embedding=embed(embedding_text),
)
store.upsert_nodes([node])

if existing is not None:
    stale_keys = frozenset(_content_from_existing_node(existing)) - frozenset(content)
    _remove_stale_properties(store, delta.node_key, stale_keys)

_add_ref_atomic(store, delta.node_key, content_hash, ref_to_remove)

Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

The function _release_ref_atomic is a pure wrapper that just calls store.structured_query with a fixed query and parameters. It adds no logic beyond that and is only called once. Inline it at the call site to reduce indirection.

Kody rule violation: Ponytail Audit

PR Code Suggestions ✨

Explore these optional code suggestions:

 if delta.action == "drop_ref":
     if existing is None:
         logger.warning("drop_ref for %s but the node no longer exists in Neo4j", delta.node_key)
         return
     old_refs = frozenset(existing.properties.get(_PROVENANCE_REFS_PROPERTY, []))
-    if previous_content_hash is None or previous_content_hash not in old_refs:
+    if previous_content_hash is None:
+        logger.warning(
+            "drop_ref for %s received None previous_content_hash -- skipping silently",
+            delta.node_key,
+        )
+        return
+    if previous_content_hash not in old_refs:
         # Already released by an earlier attempt at this same persist (the worker crashed or
         # lost its lease between this Neo4j write and the Postgres bookkeeping that follows
         # it) -- retrying is expected to be a no-op here, not a second release (tracer-bullet-01
         # "the Neo4j write... is self-correcting"). This is a snapshot-based pre-check only --
         # the actual release below is still atomic against live state either way.
         return

Persistent review updated to latest commit ece9218

Persistent review updated to latest commit 098fed2

@coderabbitai review

Persistent review updated to latest commit 4793b55